Skip to content

ManageTransactions compatible with Illuminate\DB #3358

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from

Conversation

xmaydin
Copy link

@xmaydin xmaydin commented Apr 15, 2025

Checklist

  • Add tests and ensure they pass

@xmaydin xmaydin requested a review from a team as a code owner April 15, 2025 20:57
@xmaydin xmaydin requested a review from alcaeus April 15, 2025 20:57
@alcaeus
Copy link
Member

alcaeus commented Apr 16, 2025

Thank you for your attempt at fixing this. However, we can't accept the pull request in this form for multiple reasons.

First of all, while we do need to change our method to be compatible with the overridden one, we want to do so with as little impact to users as possible. The PR here completely removes transaction options, which means that we're again breaking user code.
More importantly though, transactions in MongoDB are significantly different from those in other databases. When error occurs, the type of error determines the correct behaviour: either retry the commit (e.g. if a network error prevented us from receiving the result of the commit operation), retry the entire transaction (in case of a transient error that is expected to be resolved on the next attempt, e.g. a write conflict in a locked document), or abort the transaction entirely (e.g. because of a duplicate key error).

I'm closing this PR, but we're already working on a fix for the issue that ensures that downstream code continues to work.

@alcaeus alcaeus closed this Apr 16, 2025
@alcaeus alcaeus removed their request for review April 16, 2025 07:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants